-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support for wildcard attributes, implement data-* attribute #237
Conversation
dash/development/base_component.py
Outdated
List of Dash valid wildcard prefixes | ||
""" | ||
list_of_valid_wildcard_attr_prefixes = [] | ||
for wildcard_attr in ["data-*"]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose "aria-*"
too
This looks great, thanks @rmarren1 !
I'll take a look at plotly/dash-renderer#51 now
|
I've published a prerelease version at |
Added some comments in plotly/dash-renderer#51 (comment) that actually apply this repo |
**{ | ||
'data-string': 'multiple words', | ||
'data-number': 512, | ||
'data-none': None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This value will not be rendered in the html div.
Test is dependent on changes in plotly/dash-html-components#40 |
This is awesome, thank you! I see that to implement this, you're now checking not just for valid attributes but also for attributes that match valid wildcards, i.e. 'data-' and 'aria-'. A question - and I'm totally open to being convinced this is a bad idea - is there a reason to not simply allow any attributes, i.e. just not bother checking them? From my naive point of view that seems more future-proof. Could it be harmful somehow? Edit: thanks for the explanation @chriddyp ! |
Yeah, the main issue is that higher-level React based components (i.e. components that don't just render the HTML components) might not actually end up rendering these attributes. For example, here is the So, if we don't do validation of properties on the component-level, then the end user might think that their Re future proof - once the component author ends up supporting them, the end users will be able to upgrade their package to the new version that supports them. If they don't get errors on previous versions, then the end user might not be aware that they are working on a version that doesn't yet support these properties and they won't be aware that they need to upgrade. |
# Add the wildcard properties data-* and aria-* | ||
props.update({ | ||
k: getattr(self, k) | ||
for k in self.__dict__ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, way better 👍
'id': 'a', | ||
}, | ||
'type': 'MyComponent'} | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
|
||
input_call_count = Value('i', 0) | ||
|
||
@app.callback(Output('output-1', 'data-cb'), [Input('input', 'value')]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very cool that this works
Very, very nice @rmarren1 🎉
|
This is down by the way" https://unpkg.com/dash-html-components@0.11.0rc1/dash_html_components/bundle.js |
@rmarren1 - Thank you! This is really great stuff. I'm 👍 with these changes. Is there anything else that you would like to add? Otherwise, I will merge and release. |
I'm good with it, go ahead! |
@chriddyp to get it working I needed to do both |
I needed |
I'm using So far as I can tell the fix is to use |
@claresloggett, one workaround for using |
Try |
@rmarren1 thank you, those versions are working for me! \o/ @ned2 that's good advice thanks, it's a workaround for other issues too. I had seen it, just thought I'd check if I really needed to go down that route. For others' reference there is also an open PR to support more flexible JS and CSS inclusion: #171 |
Adds the data-* html attributes, like so:
Tested using Tox, however I
python -m unittest tests.test_react
test, since this is broken in Tox tests fail on master branch #236 and Events are broken; base_component.parse_events needs updating #232python -m unittest tests.development.test_base_component
andpython -m unittest tests.development.test_component_loader
tests, since only runningtox
was not causing these tests to run (or are are these tests generally invoked in another way?)